-
Notifications
You must be signed in to change notification settings - Fork 158
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
* FPS: BA2004.EnableSecureSourceCodeHashing
now will no longer generate false positives for UWP App regarding dummy.obj
.
#987
Conversation
…rate false positives for UWP App regarding `dummy.obj`.
@@ -17,6 +17,7 @@ | |||
## UNRELEASED | |||
* DEP: Update `Sarif.Sdk` submodule from [bc8cb57 to fd6e615](https://github.com/microsoft/sarif-sdk/compare/bc8cb57...fd6e615). Reference [SARIF SDK Release History](https://github.com/microsoft/sarif-sdk/blob/fd6e615/ReleaseHistory.md). | |||
* NEW: Add `--disable-telemetry` argument to disable telemetry collection. | |||
* FPS: `BA2004.EnableSecureSourceCodeHashing` will now no longer generate false positives for Universal Windows Platform (UWP) app regarding `dummy.obj`. [#976](https://github.com/microsoft/binskim/pull/976) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: FPS should come after BUG in the standard order.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@stacywray I don't think that list is ordered. Alphabetical ordering makes sense, but why should BUG be prioritized over FPS otherwise?
@@ -297,5 +302,8 @@ public IEnumerable<IOption> GetOptions() | |||
// RequiredCompilerWarnings, | |||
}.ToImmutableArray(); | |||
} | |||
|
|||
internal static bool IsLikelyUwpDummyObj(Language language, string library, string name) => | |||
language == Language.MASM && library?.Equals(name) == true && library == @"c:\dummy.obj"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
string.Equals call should specify StringComparison, presumably .Ordinal.
Drop "== true"
Don't use == comparison with strings (language and library). Use .Equals with a comparison specified.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed.
can not Drop "== true" because there is null check. But I moved it out now so more clear.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM.
@@ -297,5 +302,11 @@ public IEnumerable<IOption> GetOptions() | |||
// RequiredCompilerWarnings, | |||
}.ToImmutableArray(); | |||
} | |||
|
|||
internal static bool IsLikelyUwpDummyObj(Language language, string library, string name) => |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, this is so much more readable with the improved wrapping.
If you want to change the method name to IsUwpDummyObj --well, I think it's more than 'likely' given all these checks. :) (Or 'Presumed' if you want to be super cautious.) Up to you though. If you decide to make that change let me know and I'll restamp quickly.
@@ -17,6 +17,7 @@ | |||
## UNRELEASED | |||
* DEP: Update `Sarif.Sdk` submodule from [bc8cb57 to fd6e615](https://github.com/microsoft/sarif-sdk/compare/bc8cb57...fd6e615). Reference [SARIF SDK Release History](https://github.com/microsoft/sarif-sdk/blob/fd6e615/ReleaseHistory.md). | |||
* NEW: Add `--disable-telemetry` argument to disable telemetry collection. | |||
* FPS: `BA2004.EnableSecureSourceCodeHashing` will now no longer generate false positives for Universal Windows Platform (UWP) app regarding `dummy.obj`. [#976](https://github.com/microsoft/binskim/pull/976) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@stacywray I don't think that list is ordered. Alphabetical ordering makes sense, but why should BUG be prioritized over FPS otherwise?
public void IsLikelyUwpDummyObjTests() | ||
{ | ||
EnableSecureSourceCodeHashing.IsLikelyUwpDummyObj(Language.MASM, @"c:\dummy.obj", @"c:\dummy.obj").Should().BeTrue(); | ||
EnableSecureSourceCodeHashing.IsLikelyUwpDummyObj(Language.MASM, @"d:\dummy.obj", @"d:\dummy.obj").Should().BeFalse(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@shaopeng-gh To make sure I understand this correctly, d:\dummy.obj
should result in false
? The drive letter must be c
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes I believe the official one should have the path fixed as c:\dummy.obj
BA2004.EnableSecureSourceCodeHashing
now will no longer generate false positives for UWP App regardingdummy.obj
.Please see details in #983